[XS][1648] Add noWrap to team name display in TeamCharacterSelector
#2518
Conversation
WalkthroughThe changes made to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[frontend] [Thu Oct 24 02:25:51 UTC 2024] - Deployed 8cd2bbe to https://genshin-optimizer-prs.github.io/pr/2518/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Oct 24 21:56:16 UTC 2024] - Deployed 28fd2c6 to https://genshin-optimizer-prs.github.io/pr/2518/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sat Oct 26 23:33:41 UTC 2024] - Deployed 00a5605 to https://genshin-optimizer-prs.github.io/pr/2518/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Sun Oct 27 01:18:43 UTC 2024] - Deployed 4001c29 to https://genshin-optimizer-prs.github.io/pr/2518/frontend (Takes 3-5 minutes after this completes to be available) [Sun Oct 27 19:18:44 UTC 2024] - Deleted deployment |
There was a problem hiding this comment.
I think there is another location to apply this change, according to the issue:
https://github.com/frzyc/genshin-optimizer/blob/master/libs/gi/page-team/src/CharacterDisplay/Content.tsx#L168
| @@ -146,17 +152,19 @@ export default function TeamCharacterSelector({ | |||
| }} | |||
| > | |||
| <Typography | |||
There was a problem hiding this comment.
I think we can simplify the hierarchy:
- CardContent (flex, gap, align+justify:center, hover)
- TeamIcon
- Typography ( nowrap, h5, textshadow)
absolutely, i wanted to verify the approach before adding to other spots. let me know if this all looks right or if there are other spots you are thinking of (new to this app) |
I wonder what happens when you have both a long loadout name and a long build name? |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- libs/gi/page-team/src/CharacterDisplay/Content.tsx (1 hunks)
- libs/gi/page-team/src/LoadoutDropdown.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/gi/page-team/src/LoadoutDropdown.tsx
🔇 Additional comments (1)
libs/gi/page-team/src/CharacterDisplay/Content.tsx (1)
176-176: LGTM: Proper vertical alignment addedThe addition of
alignItems: 'center'ensures proper vertical alignment of content within the card.
| <Typography | ||
| variant="h6" | ||
| noWrap | ||
| sx={{ | ||
| gap: 1, | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| textShadow: '#000 0 0 10px !important', | ||
| }} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider flexible width distribution for long names
While the noWrap property prevents text wrapping as intended, consider adding flexible width distribution between character name and build name to handle cases where both are long.
Consider applying this improvement:
<Typography
variant="h6"
noWrap
sx={{
gap: 1,
alignItems: 'center',
justifyContent: 'center',
textShadow: '#000 0 0 10px !important',
+ flexShrink: 1,
+ minWidth: 0,
+ overflow: 'hidden',
+ textOverflow: 'ellipsis'
}}
>This change would:
- Allow the Typography to shrink when space is limited
- Ensure text truncation with ellipsis when content overflows
- Maintain the desired single-line display
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Typography | |
| variant="h6" | |
| noWrap | |
| sx={{ | |
| gap: 1, | |
| alignItems: 'center', | |
| justifyContent: 'center', | |
| textShadow: '#000 0 0 10px !important', | |
| }} | |
| > | |
| <Typography | |
| variant="h6" | |
| noWrap | |
| sx={{ | |
| gap: 1, | |
| alignItems: 'center', | |
| justifyContent: 'center', | |
| textShadow: '#000 0 0 10px !important', | |
| flexShrink: 1, | |
| minWidth: 0, | |
| overflow: 'hidden', | |
| textOverflow: 'ellipsis' | |
| }} | |
| > |
| <PersonIcon /> | ||
| <Typography | ||
| variant="h6" | ||
| noWrap | ||
| sx={{ | ||
| display: 'flex', | ||
| gap: 1, | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| textShadow: '#000 0 0 10px !important', | ||
| }} | ||
| > | ||
| <PersonIcon /> | ||
| <strong>{teamChar.name}</strong> | ||
| <Divider orientation="vertical" variant="middle" flexItem /> | ||
| <CheckroomIcon /> | ||
| </Typography> |
There was a problem hiding this comment.
💡 Codebase verification
Add maxWidth and text overflow handling for character names
The Typography component with noWrap lacks maxWidth constraints and text overflow handling, which could lead to layout issues with long character names. Add the following properties to the Typography's sx prop:
- maxWidth
- textOverflow: 'ellipsis'
- overflow: 'hidden'
🔗 Analysis chain
Verify text overflow handling for long character names
The addition of noWrap prevents text wrapping as intended. However, we should ensure proper text truncation for long character names.
Let's verify the text overflow behavior:
Let me check the surrounding context of the Typography component to better understand the layout constraints.
Let me try a different approach to check the component's structure and styling.
Let me check the parent container's styling to understand the width constraints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any maxWidth constraints or text overflow handling in the codebase
rg -g '*.tsx' -g '*.ts' 'textOverflow|maxWidth.*Typography' ./libs/gi/page-team/
Length of output: 166
Script:
#!/bin/bash
# Check the parent container's styling and width constraints
rg -g '*.tsx' -A 10 -B 10 '<Typography.*variant="h6".*noWrap' ./libs/gi/page-team/src/
Length of output: 88
Script:
#!/bin/bash
# Get the full content of CharacterDisplay/Content.tsx to understand the layout context
rg -g 'Content.tsx' -A 5 -B 5 'PersonIcon' ./libs/gi/page-team/src/CharacterDisplay/
Length of output: 2880
Script:
#!/bin/bash
# Get more context around the Typography component to see the parent container
rg -g 'Content.tsx' -A 20 -B 20 'variant="h6".*noWrap' ./libs/gi/page-team/src/CharacterDisplay/
Length of output: 96
fixed! new to this app thanks for informing.
i tried to use typography nowrap for the buildcard, didnt quite turn out how i like lol
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- libs/gi/ui/src/components/build/BuildCard.tsx (1 hunks)
🔇 Additional comments (1)
libs/gi/ui/src/components/build/BuildCard.tsx (1)
49-53: Verify text overflow behavior with long build names.The addition of
noWrapaligns with the PR objectives. However, ensure that the text overflow behavior works as expected with extremely long build names.Let's verify the maximum build name length in the codebase:
✅ Verification successful
Text overflow handling is appropriate for the component's usage
Based on the codebase analysis:
- The BuildCard component is used in simplified build views where names are passed directly as props
- The component is used in a controlled context within character editors
- The
noWrapwith ellipsis is an appropriate solution as:
- It maintains consistent card heights in the layout
- It prevents layout shifts from multi-line names
- It preserves the visual hierarchy in the card header
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find the longest build names in the codebase to verify overflow behavior # Look for build name assignments or definitions rg -U "name['\"]?\s*[:=]\s*['\"]([^'\"]*)['\"" --multilineLength of output: 195
Script:
#!/bin/bash # Let's try a different approach to find build name patterns # Search for name prop usage in BuildCard components rg -U "name.*=.*" "BuildCard" -A 2 -B 2 # Also search for test files that might contain build name examples fd "build.*test|test.*build" -e ts -e tsx -e js -e jsx --exec rg -l "name.*=.*" # Look for mock data or fixtures related to builds fd "mock|fixture" --exec rg -l "build.*name|name.*build"Length of output: 230
Script:
#!/bin/bash # Search for BuildCard component usage in the codebase rg -U "BuildCard.*name.*=.*" -A 2 -B 2 # Look for the actual BuildCard component file fd "BuildCard.tsx" # Search for test files containing BuildCard rg "BuildCard" -t testLength of output: 1056
| title={ | ||
| <Typography noWrap gutterBottom variant="h6"> | ||
| {name} | ||
| </Typography> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a title attribute for accessibility.
When text is truncated with noWrap, the full text should be accessible via tooltip for users who need to see the complete build name.
<Typography noWrap gutterBottom variant="h6"
+ title={typeof name === 'string' ? name : undefined}
>
{name}
</Typography>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| title={ | |
| <Typography noWrap gutterBottom variant="h6"> | |
| {name} | |
| </Typography> | |
| } | |
| title={ | |
| <Typography noWrap gutterBottom variant="h6" | |
| title={typeof name === 'string' ? name : undefined} | |
| > | |
| {name} | |
| </Typography> | |
| } |
Describe your changes
My first PR in this repo. Not familiar with mUI so bear with me. I just added a noWrap to a typography element, then added a nested typography element (is this ok?). i needed a nested element in order to keep using flex on the parent element, as ellipses wrap only works for inline display
willing to add more to other relevant areas when i hear that this is an acceptable pattern for approach.
Issue or discord link
Resolves #1648
Testing/validation
Screen.Recording.2024-10-23.at.10.20.22.PM.mov
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
New Features
Bug Fixes
Refactor